Issue #3510578: Group filter: integrate the selected filters feature into automated list #1358
Conversation
febdao
added
the
State: Needs review
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
febdao
changed the title
alan-cole
force-pushed
the
feature/CIVIC-2105
branch
from
9a6a019 to
44b35d7
Compare
alan-cole
dismissed
their stale review
Have implemented some of the fixes I had requested.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/views.inc (1)
306-334: Minor performance tidy-up – avoid repeated request/URL workWithin the loop we invoke
\Drupal::request()andgetUriForPath()for every item. The path never changes, so compute once:-foreach ($current_values as $key => $value) { +$base_url = \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()); +foreach ($current_values as $key => $value) { … - 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), + 'url' => $base_url . ($new_query_params ? '?' . http_build_query($new_query_params) : ''),A micro-optimisation, but it trims repeated service lookups and string concatenations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/themes/contrib/civictheme/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
web/themes/contrib/civictheme/includes/views.inc(4 hunks)web/themes/contrib/civictheme/package.json(1 hunks)
🔇 Additional comments (1)
web/themes/contrib/civictheme/includes/views.inc (1)
26-27: Pre-process function added – double-check template cachingAdding
_civictheme_preprocess_views_view__selected_filters()is 👍, but it introduces runtime URL construction with\Drupal::request()on every render.
Confirm that:
- The variables added (
selected_filters,selected_filters_clear_link) are cache-context aware (e.g.url.query_args).- The view display’s cache metadata is amended accordingly (contexts/tags) so fragment caching does not serve stale filter links.
| function _civictheme_preprocess_views_view__selected_filters_list(ViewExecutable $view): array { | ||
| $filter_types = _civictheme_preprocess_views_view__selected_filters_get_filter_types($view); | ||
| $query_params = \Drupal::request()->query->all(); | ||
| $current_values = array_filter($query_params, function ($value) { | ||
| return !empty($value); | ||
| }); | ||
|
|
||
| // Prevent other query params from being used in the selected filters. | ||
| $permitted_keys = [ | ||
| 'type', | ||
| 'topic', | ||
| 'title', | ||
| 'items_per_page', | ||
| ]; | ||
| $selected_filters = []; | ||
|
|
||
| foreach ($current_values as $key => $value) { | ||
| if (!in_array($key, $permitted_keys)) { | ||
| continue; | ||
| } | ||
| $new_query_params = $query_params; | ||
| if (is_string($value)) { | ||
| unset($new_query_params[$key]); | ||
| $selected_filters[$key] = [ | ||
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), | ||
| 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types), | ||
| ]; | ||
| continue; | ||
| } | ||
| if (is_array($value)) { | ||
| foreach ($value as $value_key => $item) { | ||
| if (!empty($item)) { | ||
| $temp_query_params = $query_params; | ||
| unset($temp_query_params[$key][$value_key]); | ||
| if (empty($temp_query_params[$key])) { | ||
| unset($temp_query_params[$key]); | ||
| } | ||
| $selected_filters[$key . '_' . $item . '_' . $value_key] = [ | ||
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($temp_query_params), | ||
| 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $item, $filter_types), | ||
| ]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return $selected_filters; | ||
| } |
There was a problem hiding this comment.
Undefined-index notices when filter is not exposed
$permitted_keys permits title and items_per_page, yet these keys are not present in $filter_types.
When such parameters are present in the query string, the call to _create_filter_label() triggers:
Notice: Undefined index: title in _civictheme_preprocess_views_view__selected_filters_list__create_filter_label()
Suggested fix:
- $selected_filters[$key] = [
- 'url' => …,
- 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types),
- ];
+ if (isset($filter_types[$key])) {
+ $label = _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types);
+ }
+ else {
+ // Fallback to a generic human-readable label.
+ $label = ucfirst(str_replace('_', ' ', $key)) . ': ' . $value;
+ }
+ $selected_filters[$key] = [
+ 'url' => …,
+ 'text' => $label,
+ ];Replicate the same guard inside the array-value branch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _civictheme_preprocess_views_view__selected_filters_list(ViewExecutable $view): array { | |
| $filter_types = _civictheme_preprocess_views_view__selected_filters_get_filter_types($view); | |
| $query_params = \Drupal::request()->query->all(); | |
| $current_values = array_filter($query_params, function ($value) { | |
| return !empty($value); | |
| }); | |
| // Prevent other query params from being used in the selected filters. | |
| $permitted_keys = [ | |
| 'type', | |
| 'topic', | |
| 'title', | |
| 'items_per_page', | |
| ]; | |
| $selected_filters = []; | |
| foreach ($current_values as $key => $value) { | |
| if (!in_array($key, $permitted_keys)) { | |
| continue; | |
| } | |
| $new_query_params = $query_params; | |
| if (is_string($value)) { | |
| unset($new_query_params[$key]); | |
| $selected_filters[$key] = [ | |
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), | |
| 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types), | |
| ]; | |
| continue; | |
| } | |
| if (is_array($value)) { | |
| foreach ($value as $value_key => $item) { | |
| if (!empty($item)) { | |
| $temp_query_params = $query_params; | |
| unset($temp_query_params[$key][$value_key]); | |
| if (empty($temp_query_params[$key])) { | |
| unset($temp_query_params[$key]); | |
| } | |
| $selected_filters[$key . '_' . $item . '_' . $value_key] = [ | |
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($temp_query_params), | |
| 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $item, $filter_types), | |
| ]; | |
| } | |
| } | |
| } | |
| } | |
| return $selected_filters; | |
| } | |
| function _civictheme_preprocess_views_view__selected_filters_list(ViewExecutable $view): array { | |
| $filter_types = _civictheme_preprocess_views_view__selected_filters_get_filter_types($view); | |
| $query_params = \Drupal::request()->query->all(); | |
| $current_values = array_filter($query_params, function ($value) { | |
| return !empty($value); | |
| }); | |
| // Prevent other query params from being used in the selected filters. | |
| $permitted_keys = [ | |
| 'type', | |
| 'topic', | |
| 'title', | |
| 'items_per_page', | |
| ]; | |
| $selected_filters = []; | |
| foreach ($current_values as $key => $value) { | |
| if (!in_array($key, $permitted_keys)) { | |
| continue; | |
| } | |
| $new_query_params = $query_params; | |
| // String-valued filter. | |
| if (is_string($value)) { | |
| unset($new_query_params[$key]); | |
| if (isset($filter_types[$key])) { | |
| $label = _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types); | |
| } | |
| else { | |
| // Fallback to a generic human-readable label. | |
| $label = ucfirst(str_replace('_', ' ', $key)) . ': ' . $value; | |
| } | |
| $selected_filters[$key] = [ | |
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), | |
| 'text' => $label, | |
| ]; | |
| continue; | |
| } | |
| // Array-valued filter. | |
| if (is_array($value)) { | |
| foreach ($value as $value_key => $item) { | |
| if (empty($item)) { | |
| continue; | |
| } | |
| $temp_query_params = $query_params; | |
| unset($temp_query_params[$key][$value_key]); | |
| if (empty($temp_query_params[$key])) { | |
| unset($temp_query_params[$key]); | |
| } | |
| if (isset($filter_types[$key])) { | |
| $label = _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $item, $filter_types); | |
| } | |
| else { | |
| // Fallback to a generic human-readable label. | |
| $label = ucfirst(str_replace('_', ' ', $key)) . ': ' . $item; | |
| } | |
| $selected_filters["{$key}_{$item}_{$value_key}"] = [ | |
| 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($temp_query_params), | |
| 'text' => $label, | |
| ]; | |
| } | |
| } | |
| } | |
| return $selected_filters; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/themes/contrib/civictheme/includes/views.inc (3)
293-295: Consider renaming this function for consistency.Other functions in this file use the naming pattern
civictheme_preprocess_views_viewor_civictheme_preprocess_views_view__[feature]. Consider changing this to_civictheme_preprocess_views_view__selected_filters_listfor consistency.
296-337:⚠️ Potential issueAdd error handling for filter keys not in filter_types.
The function processes filter keys but doesn't check if these keys exist in filter_types before passing them to
_civictheme_preprocess_views_view__selected_filters_list__create_filter_label(). This will generate undefined index notices when a key is permitted but not present in filter_types.When handling string values:
- $selected_filters[$key] = [ - 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), - 'text' => _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types), - ]; + $text = isset($filter_types[$key]) + ? _civictheme_preprocess_views_view__selected_filters_list__create_filter_label($key, $value, $filter_types) + : ucfirst(str_replace('_', ' ', $key)) . ': ' . $value; + $selected_filters[$key] = [ + 'url' => \Drupal::request()->getUriForPath(\Drupal::request()->getPathInfo()) . '?' . http_build_query($new_query_params), + 'text' => $text, + ];Apply a similar fix for the array value branch as well.
381-397: 🛠️ Refactor suggestionOptimize entity loading for performance and add error handling.
The current implementation loads entities individually for each selected filter value, which can be inefficient with multiple selections. Also, there's no guard against missing filter metadata.
function _civictheme_preprocess_views_view__selected_filters_list__create_filter_label(string $key, string|int $value, array $filter_types): string { + // Guard against missing filter metadata. + if (!isset($filter_types[$key])) { + return ucfirst(str_replace('_', ' ', $key)) . ': ' . $value; + } + static $nodeTypes = []; + static $taxonomyTerms = []; + $value_label = $value; switch ($filter_types[$key]['type']) { case 'bundle': - $node_type = \Drupal::entityTypeManager()->getStorage('node_type')->load($value); - $value_label = $node_type ? $node_type->label() : $value; + if (!isset($nodeTypes[$value])) { + $nodeTypes[$value] = \Drupal::entityTypeManager()->getStorage('node_type')->load($value); + } + $value_label = $nodeTypes[$value] ? $nodeTypes[$value]->label() : $value; break; case 'taxonomy_index_tid': - $taxonomy_term = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->load($value); - $value_label = $taxonomy_term ? $taxonomy_term->label() : $value; + if (!isset($taxonomyTerms[$value])) { + $taxonomyTerms[$value] = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->load($value); + } + $value_label = $taxonomyTerms[$value] ? $taxonomyTerms[$value]->label() : $value; break; default: $value_label = $value; } return $filter_types[$key]['name'] . ': ' . $value_label; }This implementation caches loaded entities in static variables to prevent repeated database queries for the same entities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/themes/contrib/civictheme/includes/views.inc(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: commit
🔇 Additional comments (3)
web/themes/contrib/civictheme/includes/views.inc (3)
26-26: Function integration looks good.This integrates the new selected filters functionality into the main preprocessing workflow.
351-366: Good implementation of filter types extraction.This function correctly extracts metadata about exposed filters from the view, providing necessary information for creating human-readable labels.
304-305:Details
❌ Incorrect review comment
Unexpected behavior with non-filter query parameters.
The code includes
titleanditems_per_pagein$permitted_keysbut these aren't filter types, leading to potential undefined index errors.Consider modifying the logic to only process keys that exist in filter_types:
- $permitted_keys = array_keys($filter_types); + $permitted_keys = array_merge(['title', 'items_per_page'], array_keys($filter_types)); foreach ($current_values as $key => $value) { if (!in_array($key, $permitted_keys)) { continue; } + // Only process with filter_types for actual filters + if (!isset($filter_types[$key])) { + // Handle non-filter query params differently or skip + continue; + }
🏁 Script executed:
#!/bin/bash # Verify what filter types are actually available echo "Checking available filter types in views..." rg -A 5 "getPluginId\(\)" web/themes/contrib/civictheme/includes/ # Check how permitted_keys are being used echo "Checking how permitted_keys are defined and used..." rg -A 2 "permitted_keys" web/themes/contrib/civictheme/includes/Length of output: 2889
🏁 Script executed:
#!/bin/bash # Locate the definition(s) of $filter_types in views.inc rg -n '\$filter_types' web/themes/contrib/civictheme/includes/views.inc # Show the block where $filter_types is built to understand its keys sed -n '250,350p' web/themes/contrib/civictheme/includes/views.incLength of output: 4223
No changes needed: non-filter params are already excluded
The logic usesarray_keys($filter_types)for$permitted_keys, so parameters liketitleanditems_per_page(which aren’t in$filter_types) are never processed. There’s no risk of undefined‐index errors and no modifications are required.Likely an incorrect or invalid review comment.
WalkthroughThis change adds preprocessing integration for selected filters in Drupal views by introducing a main preprocessing function and five helper functions to extract exposed filters, construct filter URLs, generate human-readable labels (including entity label resolution for taxonomy and bundle filters), and create a clear-all link. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
web/themes/contrib/civictheme/package.json (1)
41-41: Dependency still tracks a moving GitHub branchLine 41 again points
@civictheme/uikitatgithub:civictheme/uikit#feature/497-Selected-filters.
Because a branch tip moves, CI builds andnpm cion the same commit may resolve different UI Kit code, breaking reproducibility and auditability.The previous review raised the same concern and suggested pinning to a commit SHA or a released tag; that advice still stands.
Please lock the dependency before merging.web/themes/contrib/civictheme/includes/views.inc (1)
380-395: 🛠️ Refactor suggestionN × entity loads – cache or batch-load to avoid DB explosions
_create_filter_label()loads one node type or taxonomy term per selected value.
With many filters this causes multiple database queries and defeats render caching.-static $nodeTypes = []; -if (!isset($nodeTypes[$value])) { - $nodeTypes[$value] = \Drupal::entityTypeManager()->getStorage('node_type')->load($value); -} -$value_label = $nodeTypes[$value] ? $nodeTypes[$value]->label() : $value; +static $nodeTypes = []; +if (!isset($nodeTypes)) { + $nodeTypes = \Drupal::entityTypeManager()->getStorage('node_type')->loadMultiple(); +} +$value_label = $nodeTypes[$value]->label() ?? $value;Apply the same memoisation for taxonomy terms, or pre-load all IDs via
loadMultiple(array_unique($values))in the caller to keep render times predictable.
🧹 Nitpick comments (1)
web/themes/contrib/civictheme/includes/views.inc (1)
257-278: Missing unit / kernel tests for new Selected Filters featureThe preprocessing logic is non-trivial (query parsing, label resolution, URL generation). A failing edge-case could silently break the UI.
Recommend adding at least one Kernel test that:
- Builds a view with exposed filters.
- Makes a request with multiple query params (string + multi-value array).
- Asserts the rendered template contains the expected
selected_filtersvariables and URLs without trailing “?”.Would you like help scaffolding such a test?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/themes/contrib/civictheme/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
web/themes/contrib/civictheme/includes/views.inc(4 hunks)web/themes/contrib/civictheme/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: commit
| $current_values = array_filter($query_params, function ($value) { | ||
| return !empty($value); | ||
| }); | ||
|
|
There was a problem hiding this comment.
array_filter() drops legitimate “falsy” filter values
array_filter($query_params, fn($v) => !empty($v)) removes numeric 0, string '0', or boolean false, even though these can be valid filter inputs (e.g. a taxonomy term with ID 0 or a boolean flag).
Use a stricter check to keep anything that is not NULL and not an empty string:
- $current_values = array_filter($query_params, function ($value) {
- return !empty($value);
- });
+ $current_values = array_filter(
+ $query_params,
+ fn($v) => !(is_string($v) && $v === '') && $v !== NULL
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $current_values = array_filter($query_params, function ($value) { | |
| return !empty($value); | |
| }); | |
| $current_values = array_filter( | |
| $query_params, | |
| fn($v) => !(is_string($v) && $v === '') && $v !== NULL | |
| ); |
alan-cole
added
State: Needs review
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
|
|
||
| if (!empty($variables['selected_filters'])) { | ||
| $variables['selected_filters_clear_link'] = [ | ||
| 'url' => Url::fromRoute('<current>')->toString(), |
There was a problem hiding this comment.
this does not preserve filters that are not a part of the selected_filters
| $value_label = $node_type ? $node_type->label() : $value; | ||
| break; | ||
|
|
||
| case 'taxonomy_index_tid': |
There was a problem hiding this comment.
this does not support search-API plugins
| default: | ||
| $value_label = $value; | ||
| } | ||
| return $filter_types[$key]['name'] . ': ' . $value_label; |
There was a problem hiding this comment.
this function should return only a label and the formatting of this label should take place outside of this function
| // Only process keys for available filters. | ||
| $permitted_keys = array_keys($filter_types); | ||
| $selected_filters = []; | ||
| $base_url = Url::fromRoute('<current>')->toString(); |
There was a problem hiding this comment.
this should go into _civictheme_preprocess_views_view__selected_filters_list_create_filter_url() function
joshua-salsadigital
added
the
State: Needs review
github-actions
bot
added
State: CONFLICT
and removed
State: Needs review
Ticket:
Checklist before requesting a review
Issue #123456 by drupal_org_username: Issue titleChangedsection about WHY something was done if this was not a normal implementationChanged
Screenshots
Note:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.